Skip to content

fix(utils): decode CommandError stdout safely so __str__ never raises#1316

Merged
mergify[bot] merged 1 commit intomainfrom
devs/jd/fix/stack-revision-history-fetch-resilience/decode-commanderror-stdout-safely-str-never-raises--3e3fd853
Apr 29, 2026
Merged

fix(utils): decode CommandError stdout safely so __str__ never raises#1316
mergify[bot] merged 1 commit intomainfrom
devs/jd/fix/stack-revision-history-fetch-resilience/decode-commanderror-stdout-safely-str-never-raises--3e3fd853

Conversation

@jd
Copy link
Copy Markdown
Member

@jd jd commented Apr 29, 2026

CommandError.__str__ decoded captured stdout as strict UTF-8, so any
non-UTF-8 bytes from a subprocess (legacy locales, binary diff fragments,
truncated multi-byte sequences) would turn str(error) into a
UnicodeDecodeError. Every error-formatting site is affected — the
top-level CLI handler in cli.py:104, log messages, etc. — converting
the failure into an unhandled traceback instead of a clean message.

Switch to decode(errors="replace"). Invalid bytes become the U+FFFD
replacement character; all other formatting is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

`CommandError.__str__` decoded captured stdout as strict UTF-8, so any
non-UTF-8 bytes from a subprocess (legacy locales, binary diff fragments,
truncated multi-byte sequences) would turn `str(error)` into a
`UnicodeDecodeError`. Every error-formatting site is affected — the
top-level CLI handler in `cli.py:104`, log messages, etc. — converting
the failure into an unhandled traceback instead of a clean message.

Switch to `decode(errors="replace")`. Invalid bytes become the U+FFFD
replacement character; all other formatting is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Change-Id: I3e3fd853218a6c4294b17e9529794320288124e5
Copilot AI review requested due to automatic review settings April 29, 2026 12:22
@jd
Copy link
Copy Markdown
Member Author

jd commented Apr 29, 2026

This pull request is part of a Mergify stack:

# Pull Request Link
1 fix(utils): decode CommandError stdout safely so str never raises #1316 👈
2 fix(stack): surface fetch_old_pr_heads failures instead of swallowing #1315

@mergify mergify Bot deployed to Mergify Merge Protections April 29, 2026 12:22 Active
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Apr 29, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 🤖 Continuous Integration

Wonderful, this rule succeeded.
  • all of:
    • check-success=ci-gate

🟢 👀 Review Requirements

Wonderful, this rule succeeded.
  • any of:
    • #approved-reviews-by>=2
    • author = dependabot[bot]
    • author = mergify-ci-bot
    • author = renovate[bot]

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?:

🟢 🔎 Reviews

Wonderful, this rule succeeded.
  • #changes-requested-reviews-by = 0
  • #review-requested = 0
  • #review-threads-unresolved = 0

🟢 📕 PR description

Wonderful, this rule succeeded.
  • body ~= (?ms:.{48,})

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens error formatting for subprocess failures by ensuring CommandError.__str__() cannot raise UnicodeDecodeError when captured stdout contains non‑UTF‑8 bytes, preventing secondary tracebacks in CLI/log error paths.

Changes:

  • Decode CommandError.stdout using errors="replace" to guarantee safe stringification.
  • Add a regression test covering non‑UTF‑8 stdout handling in CommandError.__str__().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
mergify_cli/utils.py Makes CommandError.__str__() robust to invalid UTF‑8 bytes by decoding with replacement.
mergify_cli/tests/test_utils.py Adds a regression test ensuring str(CommandError) doesn’t raise on non‑UTF‑8 stdout.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mergify mergify Bot requested a review from a team April 29, 2026 12:29
@jd jd marked this pull request as ready for review April 29, 2026 13:19
@mergify mergify Bot requested a review from a team April 29, 2026 13:46
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Apr 29, 2026

Merge Queue Status

This pull request spent 7 minutes 6 seconds in the queue, including 6 minutes 45 seconds running CI.

Required conditions to merge

mergify Bot added a commit that referenced this pull request Apr 29, 2026
@mergify mergify Bot added the queued label Apr 29, 2026
mergify Bot added a commit that referenced this pull request Apr 29, 2026
@mergify mergify Bot merged commit 882bafe into main Apr 29, 2026
16 checks passed
@mergify mergify Bot deleted the devs/jd/fix/stack-revision-history-fetch-resilience/decode-commanderror-stdout-safely-str-never-raises--3e3fd853 branch April 29, 2026 17:14
@mergify mergify Bot removed the queued label Apr 29, 2026
mergify Bot pushed a commit that referenced this pull request Apr 30, 2026
…#1315)

The exception handler around `fetch_old_pr_heads` previously did a
silent `pass`, leaving every revision-history entry stamped
"unknown" with no clue why. When a real failure happens (fork remote
without `refs/pull/N/head`, network error, missing permissions, …)
the user has no signal that anything went wrong — they only see the
symptom in the PR comment days later.

Log the underlying error in orange so the cause is visible at push
time. The exception text is run through `rich.markup.escape` so any
`[`/`]` in the git error message is rendered literally rather than
parsed as Rich markup. Behaviour stays non-fatal: classification
still falls back to "unknown" and the push proceeds.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Depends-On: #1316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants